Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: removing unnecessary optionals and increasing branch cov - 1 #2503

Merged
merged 7 commits into from
Oct 16, 2023

Conversation

suyashpatil78
Copy link
Contributor

@suyashpatil78 suyashpatil78 commented Oct 10, 2023

Description

🤖 Generated by Copilot at 881e2a6

Added and updated test cases for add-edit-expense page and fixed compatibility issues with older devices. The test cases cover new scenarios for transaction fields, corporate card expenses, and autofill settings. The compatibility issues were caused by the optional chaining operator in taxGroupControl.value and currencyObjControl.value expressions in add-edit-expense.page.ts.

🤖 Generated by Copilot at 881e2a6

We test the edge cases of our code
We don't rely on optional chaining
We mock the data and fix the format
We are the masters of expense tracking

Walkthrough

🤖 Generated by Copilot at 881e2a6

  • Remove optional chaining operator from taxGroupControl.value and currencyObjControl.value expressions in AddEditExpensePage class to avoid runtime errors on older devices (link)
  • Add mock data file expense-field-obj.data to import statement in add-edit-expense-1.spec.ts for testing transaction fields (link)
  • Add new test case for removeCorporateCardExpense method when etxn$ observable is undefined in add-edit-expense-1.spec.ts and expect it to call goBack method (link)
  • Add new test case for getActionSheetOptions method when project_id is defined in add-edit-expense-1.spec.ts and expect it to return six action sheet options and verify their handlers (link)
  • Add new test case for getNewExpenseObservable method when autofill settings are disabled and recently used currency is undefined in add-edit-expense-3.spec.ts and expect it to return a new expense observable with home currency and source (link)
  • Change formatting of TestBed.inject, corporateCreditCardExpenseService, transactionService, popoverController, trackingService, orgSettingsService, and launchDarklyService statements in add-edit-expense-1.spec.ts to remove trailing commas and align closing parentheses for readability and consistency (link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link)

Clickup

app.clickup.com

Code Coverage

Please add code coverage here

UI Preview

Please add screenshots for UI changes

@github-actions github-actions bot added the size/M Medium PR label Oct 10, 2023
);
expect(trackingService.showToastMessage).toHaveBeenCalledOnceWith({
ToastContent: 'Successfully removed the card details from the expense.',
});
}));

it('should go back to my expenses page if etxn is undefined', fakeAsync(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes here

done();
});

it('should get all action sheet options and call titleCasePipe transform method if project_id is defined', (done) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change here

@@ -586,15 +586,15 @@ export class AddEditExpensePage implements OnInit {
combineLatest(this.fg.controls.currencyObj.valueChanges, this.fg.controls.tax_group.valueChanges).subscribe(() => {
if (
this.fg.controls.tax_group.value &&
isNumber(taxGroupControl.value?.percentage) &&
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have checked for this.fg.controls.tax_group.value, then no need to check for nullity again

this.fg.controls.currencyObj.value
) {
const amount =
currencyObjControl.value?.amount - currencyObjControl.value?.amount / (taxGroupControl.value?.percentage + 1);
currencyObjControl.value.amount - currencyObjControl.value.amount / (taxGroupControl.value.percentage + 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have checked for this.fg.controls.currencyObj.value, then no need to check again


const formattedAmount = this.currencyService.getAmountWithCurrencyFraction(
amount,
currencyObjControl.value?.currency
currencyObjControl.value.currency
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above


let actionSheetOptions;

component.getActionSheetOptions().subscribe((res) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's name the variables with proper names based on context. res can be what is the response we get eg: like bankAccountResponse, transactionsResponse etc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here it can be actionSheetOptionsResponse

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, changing...

let actionSheetOptions;

component.getActionSheetOptions().subscribe((res) => {
actionSheetOptions = res;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we assigning res to actionSheetOptions? & I don't see it's usage

component.recentlyUsedValues$ = of(recentlyUsedRes);
fixture.detectChanges();

component.getNewExpenseObservable().subscribe((res) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

suyashpatil78 and others added 2 commits October 13, 2023 18:17
…2504)

* test: removing unnecessary optionals and increasing branch cov - 2

* pr comments

* test: removing unnecessary optionals and increasing branch cov - 3 (#2505)

* test: removing unnecessary optionals and increasing branch cov - 3

* minor

* minor

* test: removing unnecessary optionals and increasing branch cov - 4 (#2506)

* test: removing unnecessary optionals and increasing branch cov - 4

* minor
@github-actions github-actions bot added size/L Large PR and removed size/M Medium PR labels Oct 16, 2023
@github-actions
Copy link

Unit Test Coverage % values
Statements 95.86% ( 17265 / 18010 )
Branches 93.94% ( 8704 / 9265 )
Functions 94.01% ( 5329 / 5668 )
Lines 96.02% ( 16444 / 17125 )

@suyashpatil78 suyashpatil78 merged commit 8d1d20c into master Oct 16, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Large PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants